Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repair the cache pollution caused by v.tenat != clientConfig.NamespaceId #221

Merged
merged 1 commit into from
May 23, 2021

Conversation

zou2699
Copy link
Contributor

@zou2699 zou2699 commented Apr 26, 2021

当使用多个namespceId建立客户端监听配置变化时,在此处clientConfig已经不与v对应,cacheMap.Set会造成的缓存异常。

@codecov-commenter
Copy link

Codecov Report

Merging #221 (e21535c) into master (86b5705) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head e21535c differs from pull request most recent head 80e1dcf. Consider uploading reports for the commit 80e1dcf to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   52.00%   51.88%   -0.12%     
==========================================
  Files          25       25              
  Lines        1721     1721              
==========================================
- Hits          895      893       -2     
- Misses        748      749       +1     
- Partials       78       79       +1     
Impacted Files Coverage Δ
clients/config_client/config_client.go 51.71% <100.00%> (ø)
clients/naming_client/host_reator.go 56.79% <0.00%> (-2.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86b5705...80e1dcf. Read the comment docs.

@@ -428,7 +428,7 @@ func longPulling(taskId int) func() error {
}
for _, v := range initializationList {
v.isInitializing = false
cacheMap.Set(util.GetConfigCacheKey(v.dataId, v.group, clientConfig.NamespaceId), v)
cacheMap.Set(util.GetConfigCacheKey(v.dataId, v.group, v.tenant), v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better solution might be to use a separate cacheMap for each ConfigClient

Copy link
Contributor Author

@zou2699 zou2699 Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc1.NamespaceId="namespace1"
client, _ := clients.NewConfigClient(
    vo.NacosClientParam{
        ClientConfig:  cc1,
        ServerConfigs: sc,
    },
)

// key test-data-1@@test-group@@namespace1
_ = client.ListenConfig(vo.ConfigParam{
DataId:   "test-data-1",
Group:    "test-group",
OnChange: func(namespace, group, dataId, data string) {
                   //...
},
})

cc2 := new(constant.ClientConfig)
*cc2 = *cc1
cc2.NamespaceId="2"
client, _ = clients.NewConfigClient(
    vo.NacosClientParam{
        ClientConfig:  cc2,
        ServerConfigs: sc,
    },
)
// key test-data-2@@test-group@@namespace2
_ = client.ListenConfig(vo.ConfigParam{
DataId:   "test-data-2",
Group:    "test-group",
OnChange: func(namespace, group, dataId, data string) {
                   //...
},
})

/*
But cachemap keys may be 
[ test-data-1@@test-group@@namespace1,
  test-data-2@@test-group@@namespace2,
  test-data-2@@test-group@@namespace1,  // this is wrong
]
*/

Copy link
Member

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@binbin0325 binbin0325 merged commit f366398 into nacos-group:master May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants